[SPARK-31190][SQL] ScalaReflection should not erasure user defined AnyVal type by Ngone51 · Pull Request #27959 · apache/spark

Ngone51 · 2020-03-19T13:35:44Z

What changes were proposed in this pull request?

Improve ScalaReflection to only don't erasure non user defined AnyVal type, but still erasure other types, e.g. Any. And this brings two benefits:

Give better encode error message for some unsupported types, e.g. Any
Won't miss the walk path for the AnyVal type

Why are the changes needed?

Firstly, PR #15284 added encode(serializeFor/deserializeFor) support for value class, which extends AnyVal, by not erasure types. But, this also introduce a problem that when user try to encoder unsupported types, e.g. Any, it will fail on java.lang.ClassNotFoundException: scala.Any due to the reason that scala.Any doesn't erasure to java.lang.Object.

Also, in current getClassNameFromType(), it always erasure types which could missing walked path for user defined AnyVal types.

Does this PR introduce any user-facing change?

Yes. For the test below:

case class Bar(i: Any)
case class Foo(i: Bar) extends AnyVal

test() {
  implicitly[ExpressionEncoder[Foo]]
}

Before:

java.lang.ClassNotFoundException: scala.Any
 at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
 at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
 at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:355)
 ...

After:

java.lang.UnsupportedOperationException: No Encoder found for Any
 - field (class: "java.lang.Object", name: "i")
 - field (class: "org.apache.spark.sql.catalyst.encoders.Bar", name: "i")
 - root class: "org.apache.spark.sql.catalyst.encoders.Foo"
 at org.apache.spark.sql.catalyst.ScalaReflection$.$anonfun$serializerFor$1(ScalaReflection.scala:561)

How was this patch tested?

Added unit test and test manually.

srowen · 2020-03-19T16:00:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala

+    if (isSubtype(tpe, localTypeOf[AnyVal]) && !tpe.toString.startsWith("scala")) {
+      tpe
+    } else {
+      tpe.erasure


What does erasure do here?

Basically, it convert a Scala type to Java type. e.g. scala.Any -> java.lang.Object.

SparkQA · 2020-03-19T18:32:08Z

Test build #120053 has finished for PR 27959 at commit 9e6a006.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Bar(i: Any)
case class Foo(i: Bar) extends AnyVal
test(\"AnyVal class with Any fields\")

cloud-fan · 2020-03-20T05:08:39Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala

+   * or nested classes in package objects, it uses the dollar sign ($) to create
+   * synthetic classes, emulating behaviour in Java bytecode.
+   */
+  def getClassNameFromType(tpe: `Type`): String = {


do we need to quote Type?

I don't know it clearly...just copied from the original one.

cloud-fan · 2020-03-20T05:10:02Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala

-   * synthetic classes, emulating behaviour in Java bytecode.
-   */
-  def getClassNameFromType(tpe: `Type`): String = {
-    tpe.dealias.erasure.typeSymbol.asClass.fullName


Why it's a problem only for AnyVal? Here we blindly erasure everything.

For a AnVal class, e.g. case class Foo(i: Int) extends AnyVal, erasure on it will return type of Int instead of Foo. So, this actually cause a problem when we add the class name to the walkedTypePath.

cloud-fan · 2020-03-20T07:34:56Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala

  }

+  private def erasure(tpe: Type): Type = {
+    if (isSubtype(tpe, localTypeOf[AnyVal]) && !tpe.toString.startsWith("scala")) {


can we add some comments to explain why we special case AnyVal?

SparkQA · 2020-03-20T17:24:14Z

Test build #120097 has finished for PR 27959 at commit ceb2ce6.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
//case class Foo(i: Int) extends AnyValwill return typeIntinstead ofFoo.

Ngone51 · 2020-03-23T01:16:14Z

Jenkins, retest this please.

SparkQA · 2020-03-23T05:29:14Z

Test build #120172 has finished for PR 27959 at commit ceb2ce6.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
//case class Foo(i: Int) extends AnyValwill return typeIntinstead ofFoo.

cloud-fan · 2020-03-23T08:28:49Z

thanks, merging to master/3.0!

…yVal type ### What changes were proposed in this pull request? Improve `ScalaReflection` to only don't erasure non user defined `AnyVal` type, but still erasure other types, e.g. `Any`. And this brings two benefits: 1. Give better encode error message for some unsupported types, e.g. `Any` 2. Won't miss the walk path for the `AnyVal` type ### Why are the changes needed? Firstly, PR #15284 added encode(serializeFor/deserializeFor) support for value class, which extends `AnyVal`, by not erasure types. But, this also introduce a problem that when user try to encoder unsupported types, e.g. `Any`, it will fail on `java.lang.ClassNotFoundException: scala.Any` due to the reason that `scala.Any` doesn't erasure to `java.lang.Object`. Also, in current `getClassNameFromType()`, it always erasure types which could missing walked path for user defined `AnyVal` types. ### Does this PR introduce any user-facing change? Yes. For the test below: ``` case class Bar(i: Any) case class Foo(i: Bar) extends AnyVal test() { implicitly[ExpressionEncoder[Foo]] } ``` Before: ``` java.lang.ClassNotFoundException: scala.Any at java.net.URLClassLoader.findClass(URLClassLoader.java:382) at java.lang.ClassLoader.loadClass(ClassLoader.java:418) at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:355) ... ```` After: ``` java.lang.UnsupportedOperationException: No Encoder found for Any - field (class: "java.lang.Object", name: "i") - field (class: "org.apache.spark.sql.catalyst.encoders.Bar", name: "i") - root class: "org.apache.spark.sql.catalyst.encoders.Foo" at org.apache.spark.sql.catalyst.ScalaReflection$.$anonfun$serializerFor$1(ScalaReflection.scala:561) ``` ### How was this patch tested? Added unit test and test manually. Closes #27959 from Ngone51/impr_anyval. Authored-by: yi.wu <yi.wu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 5c4d44b) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Ngone51 · 2020-03-23T09:23:09Z

thanks all!

…yVal type ### What changes were proposed in this pull request? Improve `ScalaReflection` to only don't erasure non user defined `AnyVal` type, but still erasure other types, e.g. `Any`. And this brings two benefits: 1. Give better encode error message for some unsupported types, e.g. `Any` 2. Won't miss the walk path for the `AnyVal` type ### Why are the changes needed? Firstly, PR apache#15284 added encode(serializeFor/deserializeFor) support for value class, which extends `AnyVal`, by not erasure types. But, this also introduce a problem that when user try to encoder unsupported types, e.g. `Any`, it will fail on `java.lang.ClassNotFoundException: scala.Any` due to the reason that `scala.Any` doesn't erasure to `java.lang.Object`. Also, in current `getClassNameFromType()`, it always erasure types which could missing walked path for user defined `AnyVal` types. ### Does this PR introduce any user-facing change? Yes. For the test below: ``` case class Bar(i: Any) case class Foo(i: Bar) extends AnyVal test() { implicitly[ExpressionEncoder[Foo]] } ``` Before: ``` java.lang.ClassNotFoundException: scala.Any at java.net.URLClassLoader.findClass(URLClassLoader.java:382) at java.lang.ClassLoader.loadClass(ClassLoader.java:418) at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:355) ... ```` After: ``` java.lang.UnsupportedOperationException: No Encoder found for Any - field (class: "java.lang.Object", name: "i") - field (class: "org.apache.spark.sql.catalyst.encoders.Bar", name: "i") - root class: "org.apache.spark.sql.catalyst.encoders.Foo" at org.apache.spark.sql.catalyst.ScalaReflection$.$anonfun$serializerFor$1(ScalaReflection.scala:561) ``` ### How was this patch tested? Added unit test and test manually. Closes apache#27959 from Ngone51/impr_anyval. Authored-by: yi.wu <yi.wu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Ngone51 added 2 commits March 19, 2020 17:16

impr

8ad2a15

add test

9e6a006

srowen reviewed Mar 19, 2020

View reviewed changes

cloud-fan reviewed Mar 20, 2020

View reviewed changes

cloud-fan approved these changes Mar 20, 2020

View reviewed changes

add comment

ceb2ce6

cloud-fan changed the title ~~[SPARK-31190][SQL] ScalaReflection should erasure non user defined AnyVal type~~ [SPARK-31190][SQL] ScalaReflection should not erasure non user defined AnyVal type Mar 23, 2020

cloud-fan changed the title ~~[SPARK-31190][SQL] ScalaReflection should not erasure non user defined AnyVal type~~ [SPARK-31190][SQL] ScalaReflection should not erasure user defined AnyVal type Mar 23, 2020

cloud-fan closed this in 5c4d44b Mar 23, 2020

Conversation

Ngone51 commented Mar 19, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

srowen Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

Ngone51 Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 19, 2020

Uh oh!

cloud-fan Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

Ngone51 Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

Ngone51 Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 20, 2020

Uh oh!

Ngone51 commented Mar 23, 2020

Uh oh!

SparkQA commented Mar 23, 2020

Uh oh!

cloud-fan commented Mar 23, 2020

Uh oh!

Ngone51 commented Mar 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants